Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade vaadin and importmap and add caching #35712

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

phillip-kruger
Copy link
Member

This PR upgrade to the latest vaadin, that is now build with a new version of mvnpm, that allows caching of these resources.

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/vertx labels Sep 4, 2023
Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

One comment about the data formatter.

}

private String formatDate(TemporalAccessor t) {
DateTimeFormatter formatter = DateTimeFormatter.ofPattern("EEE, dd MMM yyyy HH:mm:ss z", Locale.ENGLISH)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that a bit expensive?
Isn't that the API that was made thread-safe?

@geoand do you know?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that the API that was made thread-safe

Correct.

Isn't that a bit expensive?

Yeah, it's not very performant AFAIK. Netty has DateFormatter but I don't know how it compares and if it has been optimized. @franz1981 do you know?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's an hot path probably worth using a whole different pattern...
The easier thing is to save in a static final field the formatter, which is immutable and thread safe (please check the doc yourself, I am still on PTO so I have limited GitHub access..).

The best would be to have a volatile field with the formatted string, updated using a timer (or some scheduled task) with second granularity. It's less costly and in the hot path the best solution overall.

Copy link
Contributor

@franz1981 franz1981 Sep 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As usual, measure, not guess; check if the date formatting is a costly hot path and you have few options to reduce its overhead...
@geoand I didn't checked Netty yet: will do with my beloved desktop at hand 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is Dev mode only, and also Dev UI only, so for now I think we can merge.

@quarkus-bot

This comment has been minimized.

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 5, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@phillip-kruger phillip-kruger added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 5, 2023
@phillip-kruger phillip-kruger merged commit 8c74419 into quarkusio:main Sep 5, 2023
@quarkus-bot quarkus-bot bot added this to the 3.4 - main milestone Sep 5, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/vertx
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants